Fix GH-19653: Closure named argument unpacking between temporary closures can cause a crash #19654
Conversation
…losures can cause a crash Due to closures, the `fbc` address isn't unique if the memory address is reused. So for user closures we need to distinguish using a unique key, while internal functions can't disappear and therefore can use the `fbc` address as unique key. As for performance: This adds a load+compare for the function type, although we already load and compare the function type down below, so that impact on performance should be small. It also adds a load on the opcodes array, but we access fields from the same cacheline too so that should also have only a small impact.
There was a problem hiding this comment.
For some reason, I didn't understand the problem from the explanation at the first place, but when I started writing a comment, I found that the explanation says almost the same that I liked to say :) - "fbc" can't be stored in cache, because PHP may free the closure and allocate another one (with the same address).
| /* Due to closures, the `fbc` address isn't unique if the memory address is reused. | ||
| * So for user closures we need to distinguish using a unique key, while internal functions can't disappear | ||
| * and therefore can use the `fbc` address as unique key. */ | ||
| void *unique_id = EXPECTED(fbc->type == ZEND_USER_FUNCTION) ? (void *) fbc->op_array.opcodes : (void *) fbc; |
There was a problem hiding this comment.
fbc->op_array.opcodes is not unique unless ZEND_ACC_IMMUTABLE is set. E.g.
function usage1(Closure $c) {
$c(a: 1);
}
usage1(eval('return function($a) { var_dump($a); };'));
usage1(eval('return function($b) { var_dump($b); };'));There was a problem hiding this comment.
Good (but unfortunate) find.
This should be a rare though. What about avoiding the use of a cache slot in the case ZEND_ACC_IMMUTABLE is unset?
There was a problem hiding this comment.
This also only is problematic for files and file-bound op_arrays. I.e. for anything named that's perfectly fine.
As a suggestion to avoid branching on fbc->type, you might use fbc->common.arg_info as key instead.
Avoiding the use of a cache slot when ZEND_ACC_IMMUTABLE is unset essentially disables the optimization for every closure call though, which is unfortunate (as they aren't immutable at all, only the prototype is).
There was a problem hiding this comment.
Ideally there would be a fn_flag which would indicate whether something is persistent or not.
There was a problem hiding this comment.
fbc->common.arg_info can be shared among internal functions, no?
There was a problem hiding this comment.
I suppose then it doesn't matter because functions always ahve the same argument ordering then
There was a problem hiding this comment.
Avoiding the use of a cache slot when ZEND_ACC_IMMUTABLE is unset essentially disables the optimization for every closure call though, which is unfortunate (as they aren't immutable at all, only the prototype is).
Yeah okay this is no good.
There was a problem hiding this comment.
We can check that fbc->op_array.refcount is NULL as a proxy for "op array is persistent", but that requires branching.
Due to closures, the
fbcaddress isn't unique if the memory address is reused. So for user closures we need to distinguish using a unique key, while internal functions can't disappear and therefore can use thefbcaddress as unique key.As for performance:
This adds a load+compare for the function type, although we already load and compare the function type down below, so that impact on performance should be small. It also adds a load on the opcodes array, but we access fields from the same cacheline too so that should also have only a small impact.
It can make a performance improvement for shared opcodes arrays in case of traits as the cache slot can be reused.